Move AragonApp to unstructured storage #376

bingen · 2018-07-30T19:34:06Z

Fixes #342

TODO:

~~Do the same for Kernel~~
Adapt dao kits and apps if needed

coveralls · 2018-07-31T09:57:18Z

Coverage increased (+0.02%) to 99.028% when pulling 65ce93a on unstructured_storage into 7090b7c on dev.

This reverts commit 4da3a09.

izqui

My main feedback is that the fact that we are using unstructured storage shouldn't change how one interacts with the contract. It is just an implementation detail that should be abstracted away.

izqui · 2018-08-01T10:14:39Z

contracts/apps/AppStorage.sol

+contract AppStorage is UnstructuredStorage {
+    // keccak256("IKernel.kernel")
+    bytes32 public constant kernelPosition = 0xe30296d9191ef86f01c8532636453e486db506a7be081a32b88c263fbc3a5e15;
+    // keccak256("bytes32.appIdPosition")


Should be bytes32.appId for consistency with other names.

izqui · 2018-08-01T10:18:02Z

contracts/apps/AppStorage.sol

-    uint256[95] private storageOffset; // forces App storage to start at after 100 slots
-    uint256 private offset;
+contract AppStorage is UnstructuredStorage {
+    // keccak256("IKernel.kernel")


I would remove the type from the key names as types can change in the future and they are all storing a 32-byte word anyway.

It would make sense to have the type be explicit if we were storing more complex data types as arrays, mappings or structs, but for simple values I would remove it.

I would also add an aragonOS.appStorage prefix to all keys to remove the possibility of collisions.

izqui · 2018-08-01T10:21:20Z

contracts/common/UnstructuredStorage.sol

+
+contract UnstructuredStorage {
+    // keccak256("uint256.initializationBlock"), used by Initializable
+    bytes32 public constant initializationBlockPosition = 0xd9120073d934f0c287a3a735b193740486dbba88578fd8f3cbb2b9a9b654e7ce;


Why not move to the Initializable contract directly? Same comment on the naming of the key as for the other storage values in AppStorage.

izqui · 2018-08-01T10:21:51Z

contracts/common/Initializable.sol

        _;
    }

    modifier isInitialized {
-        require(initializationBlock > 0);
+        require(getStorageUint256(initializationBlockPosition) > 0);


Should use getInitializationBlock()

izqui · 2018-08-01T10:26:42Z

contracts/apps/AppStorage.sol

+    // keccak256("bytes32.appIdPosition")
+    bytes32 public constant appIdPosition = 0x71ca50ba614d1d1a9a2433a5d0187a13e1a8c20bbe0013968dd089c71fd760eb;
+    // keccak256("address.pinnedCode"), used by Proxy Pinned
+    bytes32 public constant pinnedCodePosition = 0x0e2081dcdbb92be1037e55b39e913e70697c33a8a8b9bb01110e6f9e576089b8;


I think the storage positions are an implementation detail that doesn't need to be exposed in the public API of the contract.

For all values I would add a getter and setter so the fact that AppStorage is using UnstructuredStorage is abstracted away from people interacting with the contract or deriving it:

function kernel() public view returns (IKernel) { return IKernel(getStorageAddress(kernelPosition)); } function setKernel(IKernel newKernel) internal { setStorageAddress(kernelPosition, address(_kernel)); }

Addess PR 376 comments.

izqui

Looking good! Just some comments related with the tests mainly.

Let's wait for @sohkai's review before merging though

izqui · 2018-08-03T08:24:54Z

contracts/evmscript/EVMScriptRunner.sol

@@ -9,7 +9,7 @@ import "./IEVMScriptExecutor.sol";
 import "./IEVMScriptRegistry.sol";

 import "../apps/AppStorage.sol";
-
+import "../kernel/IKernel.sol";


No need to import

izqui · 2018-08-03T08:33:59Z

test/unstructured_storage.js

@@ -0,0 +1,61 @@
+const getContract = name => artifacts.require(name)
+
+contract('Constants', accounts => {


'Constants' -> 'Unstructured storage'

izqui · 2018-08-03T08:34:38Z

test/unstructured_storage.js

+    kernel = await getContract('Kernel').new()
+  })
+
+  it('test', async () => {


Would split into one test case per value being checked

izqui · 2018-08-03T08:38:37Z

test/unstructured_storage.js

+    //checks
+    assert.equal(
+      parseInt(await web3.eth.getStorageAt(app.address, (await app.getInitializationBlockPosition())), 16),
+      (await app.getInitializationBlock.call()).toString(),


You can check what the block number was after app.initialize() and assert equality with the value stored at the storage position.

izqui · 2018-08-03T08:41:49Z

test/unstructured_storage.js

+
+    //checks
+    assert.equal(
+      parseInt(await web3.eth.getStorageAt(app.address, (await app.getInitializationBlockPosition())), 16),


I would get the positions from test/mocks/KeccakConstants.sol rather than from 'AragonApp', even though that equality already tested in test/keccak_constants.js. Would allow to remove all the position public getters from AppStubStorage.

Well, but if we remove the public getters from AppStubStorage then tests in keccak_constants.js won't work. Here I see we are testing 2 things:

That hardcoded position is what it is intended to be (not a big deal, but better to have it, to avoid collisions)

That things are properly stored at those positions.

Testing the second one, the first one would be implicitly tested if we use positions from KeccaKConstants but I think it's better to have both for 2 reasons: 1) in case of failing, it's eaiser to know what the problem is, 2) we could be pointing at another position that has the same value (yes, unlikely, but still) and having both tests we would have more chances to catch it.
(And AppSutbStorage is just for testing, so no big deal having those getters there)

Address PR 376 comments.

sohkai

👏 Really loving this, and the tests :D.

Have a few comments that I'll address :).

sohkai · 2018-08-15T10:53:28Z

contracts/apps/AppStorage.sol

+        setStorageAddress(pinnedCodePosition, _pinnedCode);
+    }
+
+    function pinnedCode() internal view returns (address) {


I think the pinnedCode could go into AppProxyPinned, since it's only relevant there.

sohkai · 2018-08-15T10:55:47Z

contracts/common/UnstructuredStorage.sol

+pragma solidity ^0.4.18;
+
+
+contract UnstructuredStorage {


Maybe we could make this a library, and remove all public functions from it (I don't think there's a big benefit in having the getters be public).

sohkai · 2018-08-15T10:56:17Z

migrations/2_apm.js

@@ -36,7 +36,7 @@ module.exports = function (deployer, network, accounts, arts = null) {
    console.log('Deployed APM at:', apmAddr)

    const apm = getContract('APMRegistry').at(apmAddr)
-    console.log('Kernel:', await apm.kernel())
+    console.log('Kernel:', await apm.kernel.call())


Hmm, I think these .call()s are unnecessary; I'll try removing them.

Yes, they are for testrpc but I had some problems without them using geth on our dao-kits, so now I'm putting them whenever I remember, so in case we finally move our tests to geth they will work.

Hmm, weird, this should be handled by truffle (truffle-contract changes its interface based on whether the function is marked constant or not), rather than a testrpc / geth issue.

sohkai · 2018-08-16T08:41:16Z

This change should be transparent to the DAO Kits and apps.

sohkai · 2018-08-21T19:42:25Z

For deployment cost:

                              CODE DEPOSIT COST    DEPLOYED BYTES     INITIALIZATION BYTES
ACL.json                      66600 more gas       +333               0
APMRegistry.json              367800 more gas      +1839              0
AppProxyFactory.json          307800 more gas      +1539              0
AppProxyPinned.json           102000 more gas      +510               +346
AppProxyUpgradeable.json      88600 more gas       +443               +240
AppStorage.json               72600 more gas       +363               0
AragonApp.json                61800 more gas       +309               0
ENSSubdomainRegistrar.json    70800 more gas       +354               0
EVMScriptRegistry.json        72600 more gas       +363               0
EVMScriptRegistryFactory.json 307800 more gas      +1539              +363
EVMScriptRunner.json          65600 more gas       +328               0
Initializable.json            27400 more gas       +137               0
Kernel.json                   348400 more gas      +1742              0
Repo.json                     61600 more gas       +308               0

Ouch :(.

I'd say this change is worth it though.

* Fix tests after linting * Remove unnecessary .call() in tests and migrations * Make UnstructuredStorage a library * Move pinnedCode storage only to AppProxyPinned

sohkai · 2018-08-21T20:14:15Z

Current deployment costs:

                              CODE DEPOSIT COST    DEPLOYED BYTES     INITIALIZATION BYTES
ACL.json                      19200 more gas       +96                0
APMRegistry.json              235200 more gas      +1176              0
AppProxyFactory.json          225000 more gas      +1125              0
AppProxyPinned.json           57600 more gas       +288               +375
AppProxyUpgradeable.json      40800 more gas       +204               +258
AppStorage.json               19600 more gas       +98                0
AragonApp.json                11800 more gas       +59                0
ENSSubdomainRegistrar.json    23400 more gas       +117               0
EVMScriptRegistry.json        25200 more gas       +126               0
EVMScriptRegistryFactory.json 225000 more gas      +1125              +126
EVMScriptRunner.json          12600 more gas       +63                0
Initializable.json            30000 less gas       -150               -1
Kernel.json                   209800 more gas      +1049              0
Repo.json                     11600 more gas       +58                0

Move AragonApp to unstructured storage

5bf14f0

bingen added enhancement sprint: 0.5.4 labels Jul 30, 2018

bingen self-assigned this Jul 30, 2018

bingen requested review from izqui and sohkai July 30, 2018 19:34

Unstructured Storage: make Initializable independent from AppStorage

f50f08d

ßingen added 3 commits July 31, 2018 12:02

Unstructured Storage: Kernel recoveryVaultId

4da3a09

Revert "Unstructured Storage: Kernel recoveryVaultId"

16a0fef

This reverts commit 4da3a09.

Unstructured Storage: hardcode constants

b5d332d

bingen changed the title ~~[WIP] Move AragonApp to unstructured storage~~ Move AragonApp to unstructured storage Jul 31, 2018

izqui suggested changes Aug 1, 2018

View reviewed changes

bingen force-pushed the unstructured_storage branch 3 times, most recently from 1cea212 to a83d751 Compare August 2, 2018 20:20

Unstructured storage: abstract sotrage API

5766201

Addess PR 376 comments.

bingen force-pushed the unstructured_storage branch from a83d751 to 5766201 Compare August 2, 2018 20:39

izqui approved these changes Aug 3, 2018

View reviewed changes

Unstructured storage: fix tests

c992f0a

Address PR 376 comments.

sohkai added breaking-change sprint: 0.5.5 labels Aug 15, 2018

sohkai approved these changes Aug 15, 2018

View reviewed changes

sohkai added 2 commits August 16, 2018 10:33

Merge branch 'dev' into unstructured_storage

c80a710

Fix linting

dc56285

Fix tests after linting

707e63b

sohkai mentioned this pull request Aug 21, 2018

Make UnstructuredStorage a library #380

Merged

Make UnstructuredStorage a library (#380)

65ce93a

* Fix tests after linting * Remove unnecessary .call() in tests and migrations * Make UnstructuredStorage a library * Move pinnedCode storage only to AppProxyPinned

sohkai merged commit 1ee632b into dev Aug 22, 2018

sohkai deleted the unstructured_storage branch August 22, 2018 13:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move AragonApp to unstructured storage #376

Move AragonApp to unstructured storage #376

bingen commented Jul 30, 2018 •

edited by sohkai

Loading

coveralls commented Jul 31, 2018 •

edited

Loading

izqui left a comment

izqui Aug 1, 2018

izqui Aug 1, 2018

izqui Aug 1, 2018

izqui Aug 1, 2018

izqui Aug 1, 2018

izqui Aug 1, 2018

izqui left a comment

izqui Aug 3, 2018

izqui Aug 3, 2018

izqui Aug 3, 2018

izqui Aug 3, 2018

izqui Aug 3, 2018

bingen Aug 3, 2018 •

edited

Loading

sohkai left a comment

sohkai Aug 15, 2018

sohkai Aug 15, 2018

sohkai Aug 15, 2018

bingen Aug 16, 2018 •

edited

Loading

sohkai Aug 16, 2018

sohkai commented Aug 16, 2018

sohkai commented Aug 21, 2018 •

edited

Loading

sohkai commented Aug 21, 2018 •

edited

Loading

		@@ -0,0 +1,61 @@
		const getContract = name => artifacts.require(name)

		contract('Constants', accounts => {

Move AragonApp to unstructured storage #376

Move AragonApp to unstructured storage #376

Conversation

bingen commented Jul 30, 2018 • edited by sohkai Loading

coveralls commented Jul 31, 2018 • edited Loading

izqui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

izqui left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bingen Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

sohkai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bingen Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sohkai commented Aug 16, 2018

sohkai commented Aug 21, 2018 • edited Loading

sohkai commented Aug 21, 2018 • edited Loading

bingen commented Jul 30, 2018 •

edited by sohkai

Loading

coveralls commented Jul 31, 2018 •

edited

Loading

bingen Aug 3, 2018 •

edited

Loading

bingen Aug 16, 2018 •

edited

Loading

sohkai commented Aug 21, 2018 •

edited

Loading

sohkai commented Aug 21, 2018 •

edited

Loading